Implement reset functionality for control module#6
Implement reset functionality for control module#6michaelwestern1 wants to merge 3 commits intomainfrom
Conversation
|
Please resolve your merge conflicts before the review. |
alyashour
left a comment
alyashour
left a comment
There was a problem hiding this comment.
There is no implementation for handle_reset in this mr. You've added it to the header file but there is no actual reset functionality.
| <name>ap1_control</name> | ||
| <version>0.1.0</version> | ||
| <description>AP1 Control Node</description> | ||
| <maintainer email="alyashour1@gmail.com">Aly Ashour</maintainer> |
There was a problem hiding this comment.
Why did you change Ashour lmao
| <exec_depend>ap1_msgs</exec_depend> | ||
|
|
||
| <!-- Upstream format (valid for both build + exec) --> | ||
| <depend>rclcpp</depend> |
There was a problem hiding this comment.
We already have there's no need for <build_depend> and <exec_depend> also. Remove them
package.xml
Outdated
| <exec_depend>geometry_msgs</exec_depend> | ||
| <exec_depend>ap1_msgs</exec_depend> | ||
|
|
||
| <!-- Upstream format (valid for both build + exec) --> |
There was a problem hiding this comment.
These comments about the merge aren't needed.
include/ap1/control/control_node.hpp
Outdated
| #include "ap1_msgs/msg/turn_angle_stamped.hpp" | ||
| #include "ap1_msgs/msg/vehicle_speed_stamped.hpp" | ||
|
|
||
| // ---- Your Reset Service ---- |
There was a problem hiding this comment.
Remove this artifact and others. Things like (upstream) and (your ...) don't make sense to include.
include/ap1/control/control_node.hpp
Outdated
| float last_velocity_ = 0.0f; | ||
| float last_error_ = 0.0f; | ||
| float integral_term_ = 0.0f; | ||
|
|
There was a problem hiding this comment.
These states should definitely not be in the control node. They are properties of the controller (icontroller instance).
include/ap1/control/control_node.hpp
Outdated
| // Memory | ||
| // half these types are very unnecessary, we should just have stampedfloat or | ||
| // stamped double or something | ||
| // Cached “latest values” |
There was a problem hiding this comment.
What latest values? These are messages. This comment should be replaced with something clearer. There is a mr that does the modification the comment asks for so just remove it.
| ap1_msgs::msg::VehicleSpeedStamped vehicle_speed_; | ||
| ap1_msgs::msg::TurnAngleStamped vehicle_turn_angle; | ||
| ap1_msgs::msg::TurnAngleStamped vehicle_turn_angle_; | ||
|
|
| rclcpp::Publisher<ap1_msgs::msg::TurnAngleStamped>::SharedPtr turning_angle_pub_; | ||
| rclcpp::Publisher<ap1_msgs::msg::MotorPowerStamped>::SharedPtr motor_power_pub_; // between -1 and 1? probably | ||
| rclcpp::Publisher<ap1_msgs::msg::MotorPowerStamped>::SharedPtr motor_power_pub_; | ||
|
|
There was a problem hiding this comment.
why did you move the class methods to be between the subscriptions and publishers?
2 participants
Merge Request / Pull Request
Summary of Changes
Briefly describe what this MR/PR does and which issue/task it addresses.
Type of Change
Checklist (to be completed before review)
Related Issue / Task
Notes / Additional Context
Any context for reviewers (limitations, known issues, future work)